-
Notifications
You must be signed in to change notification settings - Fork 121
[Analytics Hub] Add network actions #8211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Analytics Hub] Add network actions #8211
Conversation
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clear and works as described! 👍
Results are stored in existing
@publishedvars without any UI binding yet.
Just want to note that if it's more straightforward to bind the response directly to the card view models, feel free to remove those @published vars. They made more sense to me when I thought we'd be fetching from storage!
|
Thanks for the review!
Yeah, maybe we can remove them, but let's make it a scope of a different UI-focused PR. |
Ecarrion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a review, feel free to merge with the above approval 😅
| do { | ||
| try await retrieveOrderStats() | ||
| } catch { | ||
| DDLogWarn("⚠️ Error fetching analytics data: \(error)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a task to handle these errors later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we have #8213!
| func retrieveOrderStats() async throws { | ||
| // TODO: get dates from the selected period | ||
| let currentMonthDate = Date() | ||
| let previousMonthDate = Calendar.current.date(byAdding: .month, value: -1, to: Date())! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is temporary right? The ! concerned me 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, dates should come from #8193 🙂
You can test the changes from this Pull Request by:
|
Part of #8189
Description
This PR adds 2 network requests in
AnalyticsHubViewModelthat fetch stats data for current and previous period.Input dates are hardcoded for current/previous month now, should later use input from time range selector.
Results are stored in existing @published vars without any UI binding yet.
Testing
Add a breakpoint/
print(...)in the end ofAnalyticsHubViewModels initializer, checkcurrentOrderStatsandpreviousOrderStatsvalues.RELEASE-NOTES.txtif necessary.